Conversation
Two new endpoints on the internal API: - POST /info/users/nameid — forward lookup (sho + uid + sp_entityid → nameid) - POST /info/users/id — reverse lookup (nameid → sho + uid + sp_entityid) Both require ROLE_API_USER_NAMEID_LOOKUP and are feature-flag gated.
kayjoosten
force-pushed
the
feature/1931-nameid-lookup-api
branch
from
71be884 to
2ed58fd
Compare
kayjoosten
changed the title
kayjoosten
force-pushed
the
feature/1931-nameid-lookup-api
branch
from
2ed58fd to
6367c74
Compare
config/packages/ci/parameters.yml
Outdated
| api.users.nameIdLookup.username: nameid | ||
| api.users.nameIdLookup.password: secret | ||
| feature_api_users_nameid: true | ||
| feature_api_users_id: true |
There was a problem hiding this comment.
The original ticket does not mention feature flags. I see the deprovision controller does also use a feature flag. Did you check with Bas if we want feature flags?
I think it fits, but would be good to doublecheck.
Also, one or two feature flags?
There was a problem hiding this comment.
I have discussed with bas and we have decided that 1 feature flag will be enough so ill change that
There was a problem hiding this comment.
@kayjoosten
Still 2 feature flags?
Is this PR ready for re-review?
|
|
||
| public function resolveNameId(string $schacHomeOrganization, string $uid, string $spEntityId): ?array | ||
| { | ||
| $collabPersonId = new CollabPersonId(sprintf( |
There was a problem hiding this comment.
Use \OpenConext\EngineBlock\Authentication\Value\CollabPersonId::generateWithReplacedAtSignFrom instead?
|
|
||
| $parts = explode(':', $user->collabPersonId->getCollabPersonId(), 5); | ||
| $schacHomeOrganization = $parts[3] ?? ''; | ||
| $uid = $parts[4] ?? ''; |
There was a problem hiding this comment.
Should it fallback to ''? It should fail if something fails that should not happen.
| { | ||
| return $this->findOneBy([ | ||
| 'userUuid' => $userUuid, | ||
| 'serviceProviderUuid' => $serviceProviderUuid, |
There was a problem hiding this comment.
Nitpick: Not sure if theoretical, but what if two matching records exist? It would return a random of the existing records.
You could argue that db constraints should prevent that from occuring if that is the constraint, so ok to leave it as is.
There was a problem hiding this comment.
Duplicates for the same user+SP combination are impossible by design: persistentId is the primary key and is computed as sha1('COIN:' + userUuid + serviceProviderUuid). The same pair always produces the same SHA1, so the DB would reject a duplicate insert. findOneBy is safe here.
| { | ||
| return $this | ||
| ->createQueryBuilder('u') | ||
| ->where('u.collabPersonUuid = :uuid') |
There was a problem hiding this comment.
This is indexed, but not a unique key in the db 🤔
There was a problem hiding this comment.
You're right, idx_user_uuid on the user table is a regular index, not a UNIQUE constraint.
However, UUIDs are generated via CollabPersonUuid::generate() which uses Uuid::uuid4(). Collisions are astronomically unlikely, and the getOneOrNullResult() call would throw a NonUniqueResultException if it ever happened, making the problem visible rather than silent.
This is an application-level guarantee, same as the existing findByCollabPersonId method in the same repository which follows the same pattern.
Adding a UNIQUE constraint is a separate concern outside this PR's scope i think. What do you think?
kayjoosten
force-pushed
the
feature/1931-nameid-lookup-api
branch
from
9b34a40 to
c074c69
Compare
… flag api.users_nameid_lookup
kayjoosten
force-pushed
the
feature/1931-nameid-lookup-api
branch
from
d049fad to
c16155e
Compare
2 participants
No description provided.